Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new addon r.boxplot #743

Merged
merged 19 commits into from
May 4, 2022
Merged

new addon r.boxplot #743

merged 19 commits into from
May 4, 2022

Conversation

ecodiv
Copy link
Contributor

@ecodiv ecodiv commented Apr 30, 2022

Addon to create boxplot of raster, for whole raster of per zone of zonal raster

ecodiv added 2 commits April 30, 2022 17:01
Addon to create boxplot of raster, for whole raster of per zone of zonal raster
@neteler neteler added the new addon PR contains a new addon or issue proposes one label Apr 30, 2022
@wenzeslaus
Copy link
Member

This is a great tool!

I think the name should be different. Although it deals with visualization, it is not a display command, so perhaps r prefix. The d prefix is reserved for the tools which are using the system of display drivers and monitors and can be combined with each other. This tool is separate from that. In that context, it is similar to tool such as m.nviz.image or r.scatterplot. On the other hand, there is no standard for naming visualization tools and thinking about this one as display (d.) command in general is not wrong, so I'm wondering if there is a better way of naming things here.

@ecodiv
Copy link
Contributor Author

ecodiv commented May 1, 2022

Good point. I used this name in line with the names of d.vect.colhist and d.vect.colbp add-ons. I have no preference, but I think there should be some consistency.

@ecodiv
Copy link
Contributor Author

ecodiv commented May 1, 2022

Having thought it over a bit more, I tend to agree that using a name r.boxplot is more straightforward. @wenzeslaus, what is the best way to change this? Removing this pull request and doing a new one?

@mlennert, a similar argument could be made for d.vect.colhist and d.vect.colbp? What do you think? Would renaming them to v.colhist (or v.histogram) and v.colbp (or v.boxplot) be something to consider?

@wenzeslaus
Copy link
Member

git mv old_file new_file is enough, no need for new branch or PR. (Alternatively, renaming the files (and directories) will make git status show you what are the changes and then you git add them as git status suggests.)

@ninsbl
Copy link
Member

ninsbl commented May 1, 2022

Interesting discussion.

I wonder if it wouldnt help users to find dedicated plotting functions if that would be indicated more systematically in the module name. Either as a new group of commands (e.g. starting with p., like p.rast.boxplot) or sub-group (e.g. r.plot.poxplot, v.plot.*` ...). That would also help grouping such functions (also new, future ones).

Just thinking aloud...

That said, I think the technical concept behind the d prefix is (even) less prominent in the heads of users than it is in the heads of developers. So, my guess is that there is a bit of a trade-off regarding technical consistency vs. UX consistency...

@ecodiv
Copy link
Contributor Author

ecodiv commented May 1, 2022

I'll think I rename it for now to the proposed naming above. If at some point, the decision is made for a specific and above all consistent naming convention, it may be anyway better to do that for all concerning modules at once.

@mlennert
Copy link
Contributor

mlennert commented May 1, 2022

Having thought it over a bit more, I tend to agree that using a name r.boxplot is more straightforward. @wenzeslaus, what is the best way to change this? Removing this pull request and doing a new one?

@mlennert, a similar argument could be made for d.vect.colhist and d.vect.colbp? What do you think? Would renaming them to v.colhist (or v.histogram) and v.colbp (or v.boxplot) be something to consider?

I don't have a strong opinion on this. It really depends on whether you think that the command is first about displaying something or first and foremost about analyzing a type of map. I used d. as I felt the former to be more important as plotting is about displaying, but I can understand the argument that boxplots and histograms are as much, if not more, about analysis.

So feel free to rename any of these modules as you see fit.

@wenzeslaus
Copy link
Member

See the notebook (Binder, GitHub) or doc for grass.jupyter, an object of Map will render true display commands (one or more) into an image which you can either show in the notebook or save with a unified interface. Similarly with d.mon in command line where the image goes, e.g., to wx0 window or a PNG image. Using d.vect.colbp won't have the promised effect.

The display command and driver system is more specific than visualization (regardless of how we define the terms for ourselves in other contexts). Its equivalent outside of GRASS GIS is, for example, Matplotlib, gnuplot or GMT. In GRASS GIS, it is parallel to m.nviz.image (visualization in 3D) or ps.map (cartography outputs in PostScript).

This should be definitively communicated in a better way. Suggestions welcome.

@ecodiv ecodiv changed the title new addon d.rast.boxplot new addon r.boxplot May 2, 2022
Copy link
Contributor

@veroandreo veroandreo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool module! Thanks @ecodiv! Some minor stuff here and there

src/raster/r.boxplot/r.boxplot.html Outdated Show resolved Hide resolved
src/raster/r.boxplot/r.boxplot.html Outdated Show resolved Hide resolved
src/raster/r.boxplot/r.boxplot.html Outdated Show resolved Hide resolved
src/raster/r.boxplot/r.boxplot.html Outdated Show resolved Hide resolved
src/raster/r.boxplot/r.boxplot.py Outdated Show resolved Hide resolved
src/raster/r.boxplot/r.boxplot.html Show resolved Hide resolved
ecodiv and others added 7 commits May 3, 2022 07:46
Co-authored-by: Veronica Andreo <veroandreo@gmail.com>
Co-authored-by: Veronica Andreo <veroandreo@gmail.com>
Co-authored-by: Veronica Andreo <veroandreo@gmail.com>
moved import matplotlib after main
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new addon PR contains a new addon or issue proposes one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants